Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Add static type checking #28

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Contributor

Add static type checking: use mypy to ensure that variable and function calls are using the appropriate types.

Configure and run mypy checks in CI, including the spell checking.

Documentation:
https://mypy.readthedocs.io/en/stable/index.html

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Dec 20, 2024

@effigies I see that in this repository the style checking is being done through an explicit call to ruff in a dedicated GHA workflow, as opposed to the way it is done in nireports which I watched as the inspiration for this PR:

- name: Lint NiFreeze
run: pipx run ruff check --diff
- name: Format NiFreeze
run: pipx run ruff format --diff

vs
https://github.com/nipreps/nireports/blob/main/tox.ini#L72
https://github.com/nipreps/nireports/blob/main/.github/workflows/build_test_deploy.yml#L159
https://github.com/nipreps/nireports/actions/runs/12416179089/job/34664200380

The latter seems to be more elegant and self-contained. Is there a canonical way to do this through the NiPreps?

@jhlegarreta jhlegarreta force-pushed the AddStaticTypeChecking branch from 8baa17c to fdf477e Compare December 20, 2024 19:25
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.81%. Comparing base (6b6ba70) to head (470c510).

Files with missing lines Patch % Lines
src/nifreeze/data/pet.py 0.00% 2 Missing ⚠️
src/nifreeze/registration/ants.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
- Coverage   65.84%   65.81%   -0.04%     
==========================================
  Files          18       18              
  Lines         931      936       +5     
  Branches      119      119              
==========================================
+ Hits          613      616       +3     
- Misses        274      276       +2     
  Partials       44       44              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jhlegarreta jhlegarreta force-pushed the AddStaticTypeChecking branch 2 times, most recently from 9e3f60a to e050924 Compare December 20, 2024 20:23
@jhlegarreta
Copy link
Contributor Author

@jhlegarreta jhlegarreta force-pushed the AddStaticTypeChecking branch from e050924 to 87bad32 Compare December 21, 2024 23:29
Add static type checking: use `mypy` to ensure that variable and
function calls are using the appropriate types.

Configure and run `mypy` checks in CI.

Take advantage of the commit to include running the spell checker in the
check matrix.

Documentation:
https://mypy.readthedocs.io/en/stable/index.html
Fix spelling mistakes detected by `codespell`.

Fixes:
```
./README.rst:77: difussion ==> diffusion
./src/nifreeze/testing/simulations.py:70: pricipal ==> principal
./src/nifreeze/registration/ants.py:392: initalizing ==> initializing
spellcheck: exit 65 (0.17 seconds) /home/runner/work/nifreeze/nifreeze> codespell . pid=1810
  spellcheck: FAIL code 65 (0.27=setup[0.11]+cmd[0.17] seconds)
  evaluation failed :( (0.34 seconds)
Error: Process completed with exit code 65.
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12435719520/job/34722102640?pr=28#step:8:21
Remove empty `latex_elements` section in documentation config file.

Fixes:
```
docs/conf.py:157: error:
 Need type annotation for "latex_elements" (hint: "latex_elements: dict[<type>, <type>] = ...")  [var-annotated]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:31
Fix arguments to the `unique` function in GP error analysis plot
script: get the unique values directly from the `pandas.Series`
instance and check the length of the returned values prior to getting
the first instance.

Fixes:
```
scripts/dwi_gp_estimation_error_analysis_plot.py:92: error:
 Argument 1 to "unique" has incompatible type
 "ExtensionArray | ndarray[Any, Any]"; expected "_SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]]"  [arg-type]
scripts/dwi_gp_estimation_error_analysis_plot.py:93: error:
 Argument 1 to "unique" has incompatible type
 "ExtensionArray | ndarray[Any, Any]"; expected "_SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]]"  [arg-type]
scripts/dwi_gp_estimation_error_analysis_plot.py:94: error:
 Argument 1 to "unique" has incompatible type
```

raised for example in:
Convert list to array prior to computing mean and std dev using `NumPy`.

Fixes:
```
scripts/dwi_gp_estimation_error_analysis_plot.py:97: error:
 Argument 1 to "mean" has incompatible type
 "list[ExtensionArray | ndarray[Any, Any]]"; expected "_SupportsArray[dtype[bool_ | integer[Any] | floating[Any] | complexfloating[Any, Any]]] | _NestedSequence[_SupportsArray[dtype[bool_ | integer[Any] | floating[Any] | complexfloating[Any, Any]]]] | bool | int | float | complex | _NestedSequence[bool | int | float | complex] | _SupportsArray[dtype[object_]] | _NestedSequence[_SupportsArray[dtype[object_]]]"  [arg-type]
scripts/dwi_gp_estimation_error_analysis_plot.py:98: error:
 Argument 1 to "std" has incompatible type
 "list[ExtensionArray | ndarray[Any, Any]]"; expected "_SupportsArray[dtype[bool_ | integer[Any] | floating[Any] | complexfloating[Any, Any]]] | _NestedSequence[_SupportsArray[dtype[bool_ | integer[Any] | floating[Any] | complexfloating[Any, Any]]]] | bool | int | float | complex | _NestedSequence[bool | int | float | complex] | _SupportsArray[dtype[object_]] | _NestedSequence[_SupportsArray[dtype[object_]]]"  [arg-type]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:68
Use appropriate keyword argument names to instantiate
`SphericalKriging`: left behind in commit
nipreps/eddymotion@9e65c34

Fixes:
```
scripts/dwi_gp_estimation_simulated_signal.py:139: error:
 Unexpected keyword argument "a" for "SphericalKriging"  [call-arg]
src/nifreeze/model/gpr.py:365: note: "SphericalKriging" defined here
scripts/dwi_gp_estimation_simulated_signal.py:139: error:
 Unexpected keyword argument "lambda_s" for "SphericalKriging"  [call-arg]
src/nifreeze/model/gpr.py:365: note: "SphericalKriging" defined here
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:105
Select appropriate element in case the GPR prediction method returns a
tuple.

Fixes:
```
scripts/dwi_gp_estimation_simulated_signal.py:159: error:
 Item "tuple[ndarray[Any, Any], ...]" of
 "ndarray[Any, Any] | tuple[ndarray[Any, Any], ndarray[Any, Any], ndarray[Any, Any]] | tuple[ndarray[Any, Any], ndarray[Any, Any]]"
 has no attribute "T"  [union-attr]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:109
@jhlegarreta jhlegarreta force-pushed the AddStaticTypeChecking branch from 87bad32 to 2835169 Compare December 21, 2024 23:34
Group keyword arguments into a single dictionary.

Fixes:
```
scripts/optimize_registration.py:133: error:
 Argument "output_transform_prefix" to "generate_command" has incompatible type "str"; expected "dict[Any, Any]"  [arg-type]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:141
Use `str` instead of `Path` for `_parse_yaml_config` parameter since
`argparse` passes the argument as a string, not as a `Path`.

Fixes:
```
src/nifreeze/cli/parser.py:74: error:
 Argument "type" to "add_argument" of "_ActionsContainer" has incompatible type
 "Callable[[Path], dict[Any, Any]]"; expected "Callable[[str], Any] | FileType | str"  [arg-type]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:47
Use arrays in NumPy's `percentile` function arguments.

Fixes:
```
src/nifreeze/data/filtering.py:80: error:
 Argument 1 to "percentile" has incompatible type "ndarray[Any, dtype[number[Any] | bool_]] | ndarray[tuple[int, ...], dtype[number[Any] | bool_]]";
 expected "_SupportsArray[dtype[bool_ | integer[Any] | floating[Any]]] | _NestedSequence[_SupportsArray[dtype[bool_ | integer[Any] | floating[Any]]]] | bool | int | float | _NestedSequence[bool | int | float]"  [arg-type]
src/nifreeze/data/filtering.py:81: error:
 Argument 1 to "percentile" has incompatible type
 "ndarray[Any, dtype[number[Any] | bool_]] | ndarray[tuple[int, ...], dtype[number[Any] | bool_]]";
 expected "_SupportsArray[dtype[bool_ | integer[Any] | floating[Any]]] | _NestedSequence[_SupportsArray[dtype[bool_ | integer[Any] | floating[Any]]]] | bool | int | float | _NestedSequence[bool | int | float]"  [arg-type]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:130
List `sigma_sq` in the GP model slots.

Fixes:
```
src/nifreeze/model/_dipy.py:128: error:
 Trying to assign name "sigma_sq" that is not in "__slots__" of type "nifreeze.model._dipy.GaussianProcessModel"  [misc]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:102
Add the dimensionality to the `mask` ndarray parameter annotation.

Fixes:
```
src/nifreeze/model/_dipy.py:140: error:
 "ndarray" expects 2 type arguments, but 1 given  [type-arg]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:103
Fix type hint for `figsize` parameter: use `float` as values can be
floating point numbers.

Fixes:
```
src/nifreeze/viz/signals.py:40: error:
 Incompatible default for argument "figsize" (default has type "tuple[float, float]", argument has type "tuple[int, int]")  [assignment]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:49
Provide appropriate type hints to the `reg_target_type` parameter of the
`ants._run_registration` function. The expression is set to a tuple in
https://github.com/nipreps/nifreeze/blob/6b6ba707c47b2763c040f3d3a2ceabdcf450eefe/src/nifreeze/estimator.py#L103

Fixes:
```
src/nifreeze/registration/ants.py:459: error:
 Incompatible types in assignment (expression has type "tuple[str, str]", variable has type "str")  [assignment]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:133
Instantiate `namedtuple` using the correct syntax for `ImageFrid`:
`namedtuple` expects positional arguments for its fields, not keyword
argumentsi: define the `namedtuple` first and then instantiate it.

Fixes:
```
src/nifreeze/registration/ants.py:475: error:
 Unexpected keyword argument "shape" for "tuple"  [call-arg]
.tox/typecheck/lib/python3.12/site-packages/mypy/typeshed/stdlib/builtins.pyi:873:
 note: "tuple" defined here
src/nifreeze/registration/ants.py:475: error:
 Unexpected keyword argument "affine" for "tuple"  [call-arg]
.tox/typecheck/lib/python3.12/site-packages/mypy/typeshed/stdlib/builtins.pyi:873:
 note: "tuple" defined here
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:134
Remove type hint in overriden `diag` methods in kernel classes: it is
unsafe to override a method with a more specific argument type, as it
violates the Liskov substitution principle.

Fixes:
```
src/nifreeze/model/gpr.py:335: error:
 Argument 1 of "diag" is incompatible with supertype "Kernel";
 supertype defines the argument type as
 "Buffer | _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | bool | int | float | complex | str | bytes | _NestedSequence[bool | int | float | complex | str | bytes]"
  [override]
src/nifreeze/model/gpr.py:335: note: This violates the Liskov substitution principle
src/nifreeze/model/gpr.py:335: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
src/nifreeze/model/gpr.py:445: error:
 Argument 1 of "diag" is incompatible with supertype "Kernel";
 supertype defines the argument type as
 "Buffer | _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | bool | int | float | complex | str | bytes | _NestedSequence[bool | int | float | complex | str | bytes]"
  [override]
src/nifreeze/model/gpr.py:445: note: This violates the Liskov substitution principle
src/nifreeze/model/gpr.py:335: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:93

Documentation:
https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
Import `Bounds` from `scipy.optimize`.

Fixes:
```
src/nifreeze/model/gpr.py:32: error:
 Module "scipy.optimize._minimize" does not explicitly export attribute "Bounds"  [attr-defined]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:70
Avoid type checking for private function import statement.

Fixes:
```
src/nifreeze/model/gpr.py:215: error:
 Module "sklearn.utils.optimize" has no attribute "_check_optimize_result"  [attr-defined]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:73
@jhlegarreta jhlegarreta force-pushed the AddStaticTypeChecking branch from 2835169 to 363c6e8 Compare December 22, 2024 00:26
Remove unused `namedtuple` definition in test.

Fixes:
```
test/test_gpr.py:31: error:
 First argument to namedtuple() should be "GradientTablePatch", not "gtab"  [name-match]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:129
Use `ClassVar` for type hinting the `GaussianProcessRegressor`
`_parameter_constraints` class variable in child class.

Fixes:
```
src/nifreeze/model/gpr.py:156: error:
 Cannot override class variable (previously declared on base class "GaussianProcessRegressor") with instance variable  [misc]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:71
Annotate the `optimizer` attribute type in the DiffusionGPR GPR child
class.

Fixes:
```
src/nifreeze/model/gpr.py:234: error:
 "DiffusionGPR" has no attribute "optimizer"  [attr-defined]
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:82
@jhlegarreta jhlegarreta force-pushed the AddStaticTypeChecking branch from 363c6e8 to 470c510 Compare December 22, 2024 00:29
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Dec 22, 2024

Spent a few hours on this; solved all those warnings that I could: down to 10 8 locally using the --ignore-missing-import flag and including changes in PRs #29, #30 and #44. A few notes:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant